src: move process.binding('performance') to internalBinding #22029
Conversation
nodejs-github-bot
added
c++
|
I don't have any experience with |
|
@jdalton It's used to power https://nodejs.org/api/perf_hooks.html. |
|
Ah nice! So already exposed in a user-friendly way 🤘 |
| module.exports = { | ||
| ModuleWrap: internalBinding('module_wrap').ModuleWrap, | ||
| }; | ||
| module.exports = { internalBinding }; |
There was a problem hiding this comment.
If I am not mistaken this file is now actually obsolete.
There was a problem hiding this comment.
No it is not. This is the only way internalBinding is accessible by tests
| @@ -5,10 +5,5 @@ process.emitWarning( | |||
| 'tracked by any versioning system or deprecation process.', | |||
| 'internal/test/binding'); | |||
|
|
|||
| // These exports should be scoped as specifically as possible | |||
| // to avoid exposing APIs because even with that warning and | |||
| // this file being internal people will still try to abuse it. | |||
There was a problem hiding this comment.
Is there any reason to switch from the original idea of limiting the set of exports as much as possible here?
There was a problem hiding this comment.
This was here for a reason. I'm really kinda -1 on changing it without first exploring alternatives.
There was a problem hiding this comment.
The reason is to help keep this maintainable. We use process.binding quite a bit in tests and it's far less maintainable to expose each individual property as exports on this object. I understand the reasoning but this approach keeps this test object as simple as possible.
There was a problem hiding this comment.
For instance, process.binding() appears 157 times in test/parallel. Several of those are duplicates, but supporting all of those would mean managing a very large number of exports on internal/test/binding
There was a problem hiding this comment.
I agree with @jasnell. I do not see a different way for doing this, could you come up with a different approach @apapirovski?
There was a problem hiding this comment.
Ping @apapirovski ... there's really not a scalable way of picking and choosing the exports here so if you have a suggested alternative please let me know.
maclover7
added
the
semver-major
|
CITGM ( |
|
Ping @apapirovski again. See #22029 (comment) ^^ |
|
Given that there's been no follow up on the conversation and there are multiple approvals, I plan to go ahead and land this tomorrow if there are no further objections. |
|
as the person who added that comment about keeping the exports scoped... as we're moving everything to internalBinding it would basically become a flat object of all the different exports of all the different internal bindings. the change here makes sense imo. cc @apapirovski |
PR-URL: #22029 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
|
Landed in 9f5cc1f |
Migrate
process.binding('performance')tointernalBinding('performance')Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes